Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PECO-1803] Databricks sqlalchemy is split into this folder #1

Merged
merged 18 commits into from
Oct 22, 2024

Conversation

jprakash-db
Copy link
Collaborator

@jprakash-db jprakash-db commented Aug 20, 2024

Related Links

Main split of databricks_sql_python - databricks/databricks-sql-python#417

Description

databricks_sql_python is split into the core and databricks sqlalchemy part, the databricks sqlalchemy part is moved into this repo for controlling its release events

Tasks Completed

  • Moved the databricks_sqlalchemy into this repo
  • Made necessary code changes to the repo so that post split everything works as intended
  • Changed the pyproject.toml files to handle the dependencies properly
  • Added Github actions to run the tests using the databricks_sql_connector_core

How to Test

Testing can be done using the pytest library in the same as it is mentioned in the README.md file

@jprakash-db jprakash-db requested a review from madhav-db August 20, 2024 17:54
@jprakash-db jprakash-db self-assigned this Aug 20, 2024
@jprakash-db jprakash-db removed the request for review from madhav-db August 26, 2024 04:17
@jprakash-db jprakash-db marked this pull request as ready for review August 26, 2024 04:18
README.md Outdated

```shell
pip install databricks-sql-connector[sqlalchemy]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be databricks-sql-sqlalchemy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jackyhu-db Fixed it

README.md Outdated

```shell
pip install databricks-sql-connector[databricks_sqlalchemy]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be directly installed pip install databricks_sqlalchemy

- name: Checkout Dependency Repository
uses: actions/checkout@v3
with:
repository: jprakash-db/databricks-sql-python
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not use personal repo and it should be databricks/databricks-sqlalchemy

with:
repository: jprakash-db/databricks-sql-python
path: databricks_sql_python
ref : jprakash-db/PECO-1803
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a temporary fix to check the integration tests are passing or not. Once the main split gets published I need to modify this

python -m pip install --upgrade pip
pip3 install poetry
python3 -m venv venv
ls databricks_sql_python/databricks_sql_connector_core
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why ls databricks_sql_python/databricks_sql_connector_core, it does not exist

- name: Build Dependency Package
run: |
source venv/bin/activate
pip3 install databricks_sql_python/databricks_sql_connector_core/dist/*.whl
Copy link
Contributor

@jackyhu-db jackyhu-db Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should install via pip install requirements.txt which has the package databricks_sql_connector_core as the dependency

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But currently the databricks_sql_connector_core is not a published library and that is why all these changes are made just for testing. Once we decide how to publish I will change the integration.yml file

README.md Outdated

```shell
pip install databricks-sql-connector[alembic]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, we should use pip install databricks_sqlalchemy, do not refer to databricks-sql-connector, which may confuse the user

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

README.tests.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this file?

pyproject.toml Outdated
pytest-dotenv = "^0.5.2"

[tool.poetry.urls]
"Homepage" = "https://github.com/databricks/databricks-sql-python"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it be https://github.com/databricks/databricks-sqlalchemy?

Copy link
Contributor

@jackyhu-db jackyhu-db Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, you change the package name, it was databricks.sqlalchemy, please make sure it is backward compatible

Copy link
Contributor

@jackyhu-db jackyhu-db left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You change the package name, it was databricks.sqlalchemy, please make sure it is backward compatible. Use databricks_sqlalchemy will break existing clients

Copy link
Contributor

@jackyhu-db jackyhu-db left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you have not changed any source file under src/databricks/sqlalchemy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants